buildkite test this

/ci

buildkite test this

buildkite test this

@angorayc I added React.memo to the SecurityTourStep component, it prevents some unnecessary updates from happening: 05784f5

Hi @PhilippeOberti, I've been checking the re-renders you mentioned. It's an interesting point, the reason we see the table cells highlighted is not the DefaultCellRenderer component being re-rendered, it's the useHiddenByFlyout hook that receives an update from the useLocation() call, due to the flyout URL state change.

This hook is the only code that gets re-executed, it needs to check if the flyout "expanded" value has changed, there's no way around that if we want to have a tour anchor inside a table cells.

The good news is that the only component that updates is the GuidedOnboardingTourStep wrapper. The DefaultCellRenderer children component never receives any update when the flyout state changes. On top of that, I made a small change (2534c07) so the only GuidedOnboardingTourStep cell that updates is the one that has the anchor, the rest of the cells are rendered directly:

/ci

Hey @cnasikas yes, using the observable inside the context is not a conventional approach, I understand why you prefer the onToggleFlyout callback, but I don't think this is a very good approach either, to get what we need it would look like this:

  const [state, _dispatch] = useReducer(casesContextReducer, getInitialCasesContextState());

  const dispatch: CasesContextValueDispatch = useCallback(
    (args) => {
      _dispatch(args);
      if (
        args.type === CasesContextStoreActionsList.OPEN_CREATE_CASE_FLYOUT ||
        args.type === CasesContextStoreActionsList.OPEN_ADD_TO_CASE_MODAL
      ) {
        onToggleAddToCase({ isOpen: true });
      } else if (
        args.type === CasesContextStoreActionsList.CLOSE_CREATE_CASE_FLYOUT ||
        args.type === CasesContextStoreActionsList.CLOSE_ADD_TO_CASE_MODAL
      ) {
        onToggleAddToCase({ isOpen: false });
      }
    },
    [onToggleAddToCase]
  );

  // add `dispatch` function to the context 

This is a very specific implementation for what we need right now, it'd need to be extracted to a new "side-effects" module to make it extensible, and the onToggleAddToCase prop could cause context re-renders if it gets updated from the consumer. Also, from our side, it's much harder to use, we'd need to keep this value in a new state (isAddToCaseOpen) at the CasesContext level (applications top level), and somehow make it available to be read from the GuidedTour component, which would have some other complexities.

As an alternative, I have a third proposal:
Leave the current context alone and create a separate context to store only the state so we don't interfere with the Cases components that currently use the existing CasesContext for static information. The new state context will only be used by the new useIsAddToCaseOpen hook that is exposed from the cases plugin (more hooks like this one can be added later), so only the components that use this hook (our security GuidedTour components) will be re-rendered when the state changes. I made the changes here: 96f5924

This approach is explained in the react documentation docs. I think this is cleaner than the other 2 options, more reusable, does not mess up with the current context, and is very easy to use for us.
What do you think?

@semd I really like the new proposal. I knew there was something better but I could not figure it out. Thanks! Does the order of context providers matter? Also should we add the new context provider here x-pack/plugins/cases/public/common/mock/test_providers.tsx? I think we should not but I am curious to here your opinion.

@cnasikas

I really like the new proposal. I knew there was something better but I could not figure it out

Great! Yes, Angela and I have been discussing this code for quite a while, and the observable was my idea lol, but none of us were convinced about it. Creating a separate context for the state makes a lot more sense.

Does the order of context providers matter?

Nope. The only thing that matters is that the provider wraps the consumer at some level.

Also should we add the new context provider here x-pack/plugins/cases/public/common/mock/test_providers.tsx? I think we should not but I am curious to here your opinion.

Exactly, since the CasesProvider already renders the CasesStateContext, there's no need to do anything else.

@semd @angorayc Thank you both! I will review it again today.

@elasticmachine merge upstream

@elasticmachine merge upstream

💚 Build Succeeded

• Buildkite Build
• Commit: fc02346

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

History

• 💛 Build #212021 was flaky 1e6f738
• 💔 Build #211948 failed c80bdc8
• 💔 Build #211905 failed a5e9085
• 💚 Build #211499 succeeded 39e1468
• 💚 Build #211425 succeeded 690f59b
• 💔 Build #211176 failed 2534c07

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream